Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore erroneously removed encoding of the argument count in a generic method instantiation #98731

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

kg
Copy link
Contributor

@kg kg commented Feb 21, 2024

When IBC was removed, we accidentally removed the argument count from the encoding of generic method instantiations (it was previously being conditionally omitted when IBC was active, so it should have stayed.)

This can cause crashes in multicore jit scenarios and generally makes it not work for generic methods.

Adding this back will ensure that profiles recorded in the future will be valid, though we can't fix existing ones.

Still looking into where & how I could add regression coverage for this, but I'm not sure it's necessary.

@ghost ghost assigned kg Feb 21, 2024
@kg kg marked this pull request as ready for review February 22, 2024 00:23
@steveisok
Copy link
Member

@jeffschwMSFT who else should we add to the review? In terms of testing on CI, are multicore jit tests kicked off manually?

@kg
Copy link
Contributor Author

kg commented Feb 22, 2024

The only test coverage I found so far is https://github.com/dotnet/runtime/tree/71344cea37d84bb403d3d303b2add9e6135c539c/src/tests/baseservices/TieredCompilation and it doesn't appear to verify whether the profiled methods were actually jitted. It looks like it's meant for manual verification, presumably on a runtime build with logging turned on.

@jkotas
Copy link
Member

jkotas commented Feb 22, 2024

When IBC was removed, we accidentally removed the argument count from the encoding of generic method instantiations

Could you please share a link to the PR where this happened?

@AaronRobinsonMSFT
Copy link
Member

When IBC was removed, we accidentally removed the argument count from the encoding of generic method instantiations

Could you please share a link to the PR where this happened?

This happened in #68717

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks

@jkotas jkotas merged commit 2106224 into dotnet:main Feb 22, 2024
108 of 111 checks passed
@jkotas
Copy link
Member

jkotas commented Feb 22, 2024

Backport candidate?

@AaronRobinsonMSFT
Copy link
Member

Backport candidate?

Yes, I think so.

@AaronRobinsonMSFT
Copy link
Member

/backport to release/8.0-staging

Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8008709752

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants